Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

The new version of Regex (1.9.0) is not compatible with 1.8 #3779

Merged
merged 3 commits into from
Jul 12, 2023

Conversation

sergey-shandar
Copy link
Contributor

Description

The new version of Regex (1.9.0) is not compatible with 1.8 if we use regex::internal::* See

https://github.com/Trust-Machines/stacks-sbtc/actions/runs/5466661304/jobs/9951913250

And the fix:

https://github.com/Trust-Machines/stacks-sbtc/pull/522

Applicable issues

  • fixes #

Additional info (benefits, drawbacks, caveats)

Checklist

  • Test coverage for new or modified code paths
  • Changelog is updated
  • Required documentation changes (e.g., docs/rpc/openapi.yaml and rpc-endpoints.md for v2 endpoints, event-dispatcher.md for new events)
  • New clarity functions have corresponding PR in clarity-benchmarking repo
  • New integration test(s) added to bitcoin-tests.yml

@codecov
Copy link

codecov bot commented Jul 6, 2023

Codecov Report

Merging #3779 (26ad8bd) into master (02dabb8) will increase coverage by 0.12%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #3779      +/-   ##
==========================================
+ Coverage    0.06%    0.18%   +0.12%     
==========================================
  Files         301      302       +1     
  Lines      276555   277126     +571     
==========================================
+ Hits          179      512     +333     
- Misses     276376   276614     +238     
Impacted Files Coverage Δ
clarity/src/vm/costs/mod.rs 0.00% <ø> (ø)

... and 1 file with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@kantai
Copy link
Member

kantai commented Jul 6, 2023

Thanks for finding this @sergey-shandar --

I'd suggest just deleting that line from costs/mod.rs instead:

diff --git a/clarity/src/vm/costs/mod.rs b/clarity/src/vm/costs/mod.rs
index ee9a1a6eb..c70ab9600 100644
--- a/clarity/src/vm/costs/mod.rs
+++ b/clarity/src/vm/costs/mod.rs
@@ -18,7 +18,6 @@ use std::collections::{BTreeMap, HashMap};
 use std::convert::{TryFrom, TryInto};
 use std::{cmp, fmt};
 
-use regex::internal::Exec;
 use rusqlite::types::{FromSql, FromSqlResult, ToSql, ToSqlOutput, ValueRef};
 use serde::{Deserialize, Serialize};

The Exec method isn't used anywhere, so we can just remove that import and then we don't need to version pin regex.

clarity/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Member

@kantai kantai left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great to me

@jcnelson
Copy link
Member

jcnelson commented Jul 12, 2023 via email

@kantai
Copy link
Member

kantai commented Jul 12, 2023

Just put the full version. Humans look at the cargo.toml file to determine dependencies, not the cargo.lock. The full version removes ambiguity.

This PR doesn't touch the Cargo.toml at all right now (because there was a spurious import that could just be removed to solve the issue), and I'd suggest that it stay that way.

If you want to talk about changing the way the blockchain does version specifications in the toml file, that's probably better done in an issue for changing that in the contributing guide.

@jcnelson
Copy link
Member

jcnelson commented Jul 12, 2023 via email

@sergey-shandar
Copy link
Contributor Author

@kantai is it ok that the PR is open against the master branch? Do we sync changes from the master to the develop and next branches?

@kantai
Copy link
Member

kantai commented Jul 12, 2023

@kantai is it ok that the PR is open against the master branch?

Yes -- for changes that don't touch any code (e.g., docs only, github workflows, etc.) it's okay to merge to master. This PR, because it just removes an unused import, is okay.

Do we sync changes from the master to the develop and next branches?

Yes, changes to master should always be merged back into develop and next. There isn't an automated process for this in the github repository.

@kantai kantai merged commit d15a822 into master Jul 12, 2023
@sergey-shandar sergey-shandar deleted the sergey-shandar-patch-1 branch July 12, 2023 20:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Status: ✅ Done
Development

Successfully merging this pull request may close these issues.

3 participants